Skip to content

Add @PulsarMessage for default topic/schema #565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

onobc
Copy link
Collaborator

@onobc onobc commented Feb 6, 2024

This PR adds @PulsarMessage annotation to allow specifying default topic and/or schema info for a message class.

cc: @jonasgeiregat do you mind giving this a review as well?

alesharik and others added 2 commits February 6, 2024 11:36
This commit introduces the @PulsarTypeMapping annotation which can be
used on message classes to specify default topic and/or schema info.
* Introduce a PulsarTypeMappingRegistry to cache annotations
* Expand on the topic/schema resolver tests
* Add support for KEY_VALUE on annotation
@onobc onobc changed the title Add @PulsarTypeMapping for default topic/schema Add @PulsarTypeMapping for default topic/schema Feb 6, 2024
@jonas-grgt
Copy link
Contributor

I'll look into it tomorrow, looks like a really nice addition!

@onobc
Copy link
Collaborator Author

onobc commented Feb 6, 2024

I'll look into it tomorrow, looks like a really nice addition!

All glory goes to @alesharik

Copy link
Contributor

@jonas-grgt jonas-grgt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea of annotating your message types with a little annotation that denotes the type and even the topic. I myself have done something similar on project I've worked on. Yet, I'm not convinced about the name @PulsarTypeMapping. It clearly denotes that you are able to do some kind of type mapping yet the topic information feels shoehorned in and not the right place for an annotation called this way.

I would either propose to generify the name into something like @PulsarMessageDefinition @PulsarMessageDetails or just @PulsarMessage or split the annotation into two and introduce a seperate @PulsarTopic annotation specifically for topic resolution.

I'm missing a test that verifies that PulsarTemplate can handle @PulsarTypeMapping

@onobc
Copy link
Collaborator Author

onobc commented Feb 7, 2024

I love the idea of annotating your message types with a little annotation that denotes the type and even the topic.

Me too. It was @alesharik idea.

Yet, I'm not convinced about the name @PulsarTypeMapping.

Me neither - you know what they say about naming and computer science ;)

It clearly denotes that you are able to do some kind of type mapping yet the topic information feels shoehorned in and not the right place for an annotation called this way.

I would either propose to generify the name into something like @PulsarMessageDefinition @PulsarMessageDetails or just @PulsarMessage

Ha! The original PR was named @PulsarMessage and I steered towards @PulsarTypeMapping. I agree w/ your reasoning though and I think that @PulsarMessage reads well. I will adjust this shortly. Aleksei, your original naming was sound :)

or split the annotation into two and introduce a seperate @PulsarTopic annotation specifically for topic resolution.

I thought about this approach but it would duplicate quite a bit of the work and for now a single annotation feels right.

@onobc
Copy link
Collaborator Author

onobc commented Feb 7, 2024

I'm missing a test that verifies that PulsarTemplate can handle @PulsarTypeMapping

There is not one that explicitly tests this as the PulsarTemplate codes to the SchemaResolver interface and the assumption is that it is trusted and has been thoroughly tested itself.

@onobc onobc added the type: feature A new feature or enhacement label Feb 8, 2024
@onobc onobc added this to the 1.1.0-M1 milestone Feb 8, 2024
@onobc onobc force-pushed the add-pulsar-type-mapping branch from 9dd0e17 to 3550cbc Compare February 8, 2024 18:55
@onobc onobc changed the title Add @PulsarTypeMapping for default topic/schema Add @PulsarMessage for default topic/schema Feb 8, 2024
@onobc onobc merged commit d8524d1 into spring-projects:main Feb 8, 2024
@onobc onobc deleted the add-pulsar-type-mapping branch February 8, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature or enhacement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants